Codecov Report

Patch coverage: 56.17% and project coverage change: +0.98 🎉

Comparison is base (8a078ce) 62.59% compared to head (fb63f3e) 63.58%.

@@            Coverage Diff             @@
##             main    #1823      +/-   ##
==========================================
+ Coverage   62.59%   63.58%   +0.98%     
==========================================
  Files         107      107              
  Lines       13645    13729      +84     
==========================================
+ Hits         8541     8729     +188     
+ Misses       4298     4159     -139     
- Partials      806      841      +35     

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

On similar note like #1823 (comment).
I really do not like using $(CODE) as prerequisites -- it cannot detect file deletion (only addtition/modification).
Moreover go build, go test, golangci-lint are smart enough to know when/what re-compilation/re-test is required.
Similarly I don't see much value in these stamps, what I see is possible risk of not running recipe when is should be run.
I rather run recipe that takes 500ms all the time than risking that the recipe is not done when it really needs to be.

Thanks for the review @matejvasek , I have added filepath.ToSlash() to the filesystem symlink test in place of the conditionals, and removed all but the minimum makefile updates for discussion in a separate PR.

/approve
/lgtm

The windows test are failing on the symlink despite filepath.ToSlash(), it makes no sense.

The windows test are failing on the symlink despite filepath.ToSlash(), it makes no sense.

Shucks. I was hoping the ToSlash would do the trick. I really don't mind if they are a few bytes different, so long as they are both symlinks.

The windows test are failing on the symlink despite filepath.ToSlash(), it makes no sense.

Shucks. I was hoping the ToSlash would do the trick.

@lkingland I think I know where the issue might be.
The unit test in GH Action also regenerates zz file on Windows.
That is IMO not good because actual app do use zz file generated on Linux (because of exec perms).
This means the GH Action test is testing something that is not really happening with actual app.

Anyway... the generate/templates/main.go scrip should also fixup links here (the same way it fixes up filepaths already here ).

I really don't mind if they are a few bytes different, so long as they are both symlinks.

But these bytes are significant.

actual app do use zz file generated on Linux

Actually I am not 100% positive about this (but if we are not doing it we should be). I am only sure that we version file generated on Linux.

@lkingland tl;dr we should not suppress the tests, it really indicates that something is wrong

We need filepath.ToSlash() in:

• zip generator
• os fs
• fs test

After these 3 changes it should work. I tested it.
I know this is bug in existing code, not it this PR, sill we should fix it.

diff --git a/generate/templates/main.go b/generate/templates/main.go
index 45ec4848..f29ceb4f 100644
--- a/generate/templates/main.go
+++ b/generate/templates/main.go
@@ -80,7 +80,7 @@ func main() {
 			if err != nil {
 				return err
 			}
-			_, err = w.Write([]byte(symlinkTarget))
+			_, err = w.Write([]byte(filepath.ToSlash(symlinkTarget)))
 			return err
 		case info.Mode()&fs.ModeType == 0: // regular file
 			f, err := os.Open(path)
diff --git a/pkg/filesystem/filesystem.go b/pkg/filesystem/filesystem.go
index d43aad75..c72cf5ee 100644
--- a/pkg/filesystem/filesystem.go
+++ b/pkg/filesystem/filesystem.go
@@ -173,7 +173,11 @@ func (o osFilesystem) Stat(name string) (fs.FileInfo, error) {
 
 func (o osFilesystem) Readlink(link string) (string, error) {
 	link = filepath.FromSlash(link)
-	return os.Readlink(filepath.Join(o.root, link))
+	t, err := os.Readlink(filepath.Join(o.root, link))
+	if err != nil {
+		return "", err
+	}
+	return filepath.ToSlash(t), nil
 }
 
 // subFS exposes subdirectory of underlying FS, this is similar to `chroot`.

/approve
/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, matejvasek

The full list of commands accepted by this bot can be found here.

The pull request process is described here